feat(supervisor-middleware): add network egress middleware#2027
feat(supervisor-middleware): add network egress middleware#2027pimlock wants to merge 27 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
pimlock
force-pushed
the
1733-supervisor-middleware/pmlocek
branch
2 times, most recently
from
595191e to
97b750f
Compare
pimlock
force-pushed
the
1733-supervisor-middleware/pmlocek
branch
from
358906a to
1fbcdbc
Compare
|
🌿 Preview your docs: https://nvidia-preview-pr-2027.docs.buildwithfern.com/openshell |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/ok to test c4b0dcf |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…are outages An unreachable operator-registered middleware service previously aborted sandbox startup via a hard error in load_policy, contradicting the per-request on_error contract and the resilient live-reload path. Retry the initial connect and, on failure, degrade to the built-in registry so matched requests are governed by each config's on_error (deny for fail_closed, allow for fail_open) instead of blocking the whole sandbox. The policy poll loop now reconciles the registry on every poll while an install is pending, so a recovered service is adopted without waiting for a config change; a failed reconcile also no longer blocks unrelated policy updates. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…limit A chain entry whose binding did not resolve reported a zero body limit, which dragged the whole chain's buffer cap to zero and spuriously failed body-bearing requests over capacity even when a resolved middleware could have processed them. Exclude unresolved entries from the limit via a new DescribedChainEntry::is_resolved(); when no entry resolves, skip buffering and apply each entry's on_error directly. Also fix two parallel-test flakes found while validating the change: - Build middleware OCSF events into a Vec and assert on it directly instead of capturing through the global tracing pipeline, whose callsite-interest cache is process-global and raced under parallel runs. - Accumulate the websocket deny response until the reason marker arrives rather than assuming a single read returns the full body. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
force-pushed
the
1733-supervisor-middleware/pmlocek
branch
from
c4b0dcf to
2b7cf4e
Compare
|
/ok to test 0ef948b |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test 400ca0f |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
changed the title
|
/ok to test 1f6aec1 |
pimlock
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Label |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
| Plan startup and updates around these boundaries: | ||
|
|
||
| - Start registered services before the gateway. The gateway validates every registration during startup. | ||
| - Keep service endpoints reachable from both the gateway and sandbox supervisors. The supervisors call operator-run services directly on the request path. |
There was a problem hiding this comment.
Clarifying question - What is the behavior when a service endpoint is unexpectedly or intermittently unavailable? This would be in the case where a on gateway and supervisor startups, the service was healthy, but then becomes unavailable later. What would happen?
There was a problem hiding this comment.
This depends on the on_error setting. If it's fail_open, the request will be sent upstream, if it's fail_closed (default), it will be rejected.
| // HttpRequestTarget describes the admitted HTTP destination and request target. | ||
| message HttpRequestTarget { |
There was a problem hiding this comment.
Again just out of curiosity - are generic headers in scope here or part of the target?
There was a problem hiding this comment.
The headers are currently modelled as part of the request itself, this HttpRequestTarget is supposed to represent "where" and the rest (the "what") is in the HttpRequestEvaluation.
The reason for that split was for HttpRequestTarget to represent what the network policy operates on when making its allow/deny decision.
| /// | ||
| /// Matching is case-insensitive. Invalid or empty patterns return an error | ||
| /// instead of silently becoming a non-match. | ||
| pub fn host_matches(pattern: &str, host: &str) -> std::result::Result<bool, String> { |
There was a problem hiding this comment.
Nit: Although we're using this to match a host, this is matching a general pattern to a string should we rename the function?
I'm also wondering if it make sense to introduce a Pattern type that wraps glob::Pattern and performs pattern validation on new?
| use miette::{Result, miette}; | ||
|
|
||
| /// Binding identifier for the built-in secret redaction middleware. | ||
| pub const BUILTIN_SECRETS: &str = "openshell/secrets"; |
There was a problem hiding this comment.
If this were go, I would consider implementing this as a type that implements some "Middleware" interface. Is something similar possible / idomatic in Rust?
| fn validate_secrets_config(config: &prost_types::Struct) -> Result<()> { | ||
| let mode = config | ||
| .fields | ||
| .get("secrets") | ||
| .and_then(|value| match value.kind.as_ref() { | ||
| Some(prost_types::value::Kind::StringValue(value)) => Some(value.as_str()), | ||
| _ => None, | ||
| }) | ||
| .unwrap_or("redact"); | ||
| if mode != "redact" { | ||
| return Err(miette!( | ||
| "{BUILTIN_SECRETS} only supports config.secrets: redact in phase 1" | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
It is not clear from this function which fields the openshell/secrets config actually has. Does this further justify a concrete type? (Something like what we do for the driver-config structs?)
| Some(prost_types::value::Kind::StringValue(value)) => Some(value.as_str()), | ||
| _ => None, | ||
| }) | ||
| .unwrap_or("redact"); |
There was a problem hiding this comment.
So the default value is redact even if no secrets field is present?
|
|
||
| #[test] | ||
| fn secrets_config_defaults_to_redact() { | ||
| validate_builtin_config(BUILTIN_SECRETS, &prost_types::Struct::default()).unwrap(); |
There was a problem hiding this comment.
I don't actually see redact mentioned in this test at all.
| if endpoint.tls == "skip" | ||
| && selector_matches_host(middleware, &endpoint.host).unwrap_or(false) | ||
| { |
There was a problem hiding this comment.
[P1] Detect tls-skip conflicts against wildcard endpoints
When a tls: skip endpoint uses a wildcard host such as *.example.com and a middleware selector includes a concrete host such as api.example.com, this compares the selector against the literal endpoint pattern, so validation passes even though real traffic for api.example.com is admitted without TLS inspection and the required middleware cannot run. The conflict check needs to reason about selector/endpoint pattern overlap, not only whether the selector matches the endpoint string.
| let chain = engine.query_middleware_chain(&middleware_network_input(ctx))?; | ||
| let req = match apply_middleware_chain( | ||
| req, | ||
| client, | ||
| ctx, | ||
| chain, | ||
| engine.middleware_runner(), | ||
| engine.generation_guard(), | ||
| ) |
There was a problem hiding this comment.
It looks like L7 policy is evaluated before middleware. So I'd imagine that the middleware could modify or replace an allowed operation with one that would otherwise be denied by policy. Is this intended? If so we should clearly document this.
5 participants
Summary
Implements the first usable RFC 0009 supervisor middleware slice: proto-backed, host-selected HTTP egress middleware for
HttpRequest/pre_credentials, with both in-process built-ins and statically registered operator-run gRPC services.The implementation covers RFC 0009 Phase 1 and adds basic external-service support from Phase 2. It establishes the contract, policy plumbing, ordered chain execution, built-in secret redaction, static gateway registration, relay integration, validation before policy persistence, body limits, audit events, and user-facing configuration and operations documentation.
Tip
See example middleware implementation: #2169
Related Issue
Closes #2010
Part of #1733
Design/RFC: #1738
Changes
openshell/secretsredactor and statically registered operator-run gRPC services.network_middlewarespolicy configuration and validation, independent of the network policy rule that admits a request.Testing
mise run pre-commitpassesChecklist